Fix instantiating Vault Secret Backend during configuration #17935
Conversation
|
cc: @nathadfield (@kaxil @ash @uranusjr FYI) -> yet another instantiation of our cyclic config <-> settings <-> model <> backends relationship. |
When Secrets Backend are instantiated during configuration, not all Airlfow packages are yet imported, because they need Secret Backends. We have a weird cyclical relation between models, configuration and settins which forces us to be extra careful around configuration, settings and backends. In this case top-level import of Connections by the Vault Secret Backend triggered cyclic import problem (importing airflow models require configuration to be fully loaded and initialized) but then it could not be initialized because models needed to be imported first. The fix is to move Connections to be locally imported.
potiuk
force-pushed
the
fix-not-possible-to-import-as-backend-in-hashicorp
branch
from
047896e to
dfa6c73
Compare
| # Make sure connection is imported this way for type checking, otherwise when importing | ||
| # the backend it will get a circular dependency and fail | ||
| if TYPE_CHECKING: | ||
| from airflow.models.connection import Connection |
There was a problem hiding this comment.
This can go at the top of the file
There was a problem hiding this comment.
I actually deliberately wanted to keep it in one place, to avoid any accidental removal. It's pretty "special" case (which eventually we might be able to remove) :)
There was a problem hiding this comment.
BTW. I did not even know that you can do such statements at the class level . But it worked.
There was a problem hiding this comment.
TIL: https://docs.python.org/3/tutorial/classes.html#class-definition-syntax
In practice, the statements inside a class definition will usually be function definitions, but other statements are allowed, and sometimes useful — we’ll come back to this later. The function definitions inside a class normally have a peculiar form of argument list, dictated by the calling conventions for methods — again, this is explained later.
Perfecly legitimate use case IMHO :D ..
There was a problem hiding this comment.
Yeah no strong opinion on top or bottom :D -- just won't make a difference from code view as it is inside TYPE_CHECKING conditional
github-actions
bot
added
the
okay to merge
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
When Secrets Backend are instantiated during configuration, not
all Airlfow packages are yet imported, because they need Secret
Backends. We have a weird cyclical relation between models,
configuration and settins which forces us to be extra careful
around configuration, settings and backends.
In this case top-level import of Connections by the Vault Secret
Backend triggered cyclic import problem (importing airflow models
require configuration to be fully loaded and initialized) but then
it could not be initialized because models needed to be imported
first.
The fix is to move Connections to be locally imported.
Found during testing in #17922
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.